Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't render until initial data loaded #147

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Don't render until initial data loaded #147

wants to merge 2 commits into from

Conversation

colin-grant-work
Copy link
Contributor

What it does

Fixes #145 by not showing the main UI until the initial memory data is known.

How to test

  1. Open a Memory Inspector view from the Variables view.
  2. Observe that there is not a stage where Formik errors are shown indicating that Address and Count are required.

Review checklist

Reminder for reviewers

@colin-grant-work
Copy link
Contributor Author

There are two different approaches represented in the different commits:

  • The first commit shows a small loading spinner until the first setOptions call is received.
  • The second commit reverts that and instead passes the initial options in the HTML itself, allowing it to be retrieved when the view is initialized.

Other approaches are possible, feel free to suggest your favorite.

jreineckearm
jreineckearm approved these changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@jreineckearm jreineckearm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting and fixing this, @colin-grant-work !

Had a look at both options:

  1. Having a hard time to see the spinner appear in my setup. So, can't really say how it looks like and where it appears. Except from that, if you open an empty Memory Inspector window through the command palette, the Formik error doesn't show anymore.
  2. Works nicely, and the error is shown as expected after opening an empty window through the command palette.

Even if the code changes for the first approach seem less intrusive, I believe we should go with the second solution which is already the top commit. Hence approved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreineckearm jreineckearm jreineckearm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Form validated before initial values loaded
2 participants
@colin-grant-work @jreineckearm